Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NSG monitoring for preconfiguredNSG #3156

Merged
merged 10 commits into from
Oct 27, 2023
Merged

Add NSG monitoring for preconfiguredNSG #3156

merged 10 commits into from
Oct 27, 2023

Conversation

nwnt
Copy link
Contributor

@nwnt nwnt commented Sep 7, 2023

Which issue this PR addresses:

Fixes: ARO-3391

What this PR does / why we need it:

Adding monitoring for NSG and emitting metrics when invalid NSG configurations have been detected, including when the FP (the one performing the monitoring) loses access to the NSG.

Test plan for issue:

How did you test that this PR works?

  • Unit tests are added
  • More tests to be performed with INT the environment.

Is there any documentation that needs to be updated for this PR?

N/A

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

Please rebase pull request.

@github-actions github-actions bot added the needs-rebase branch needs a rebase label Sep 7, 2023
pkg/monitor/worker.go Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the needs-rebase branch needs a rebase label Sep 8, 2023
@nwnt nwnt force-pushed the nsg-monitoring branch 4 times, most recently from aceb852 to 2a6a52b Compare September 12, 2023 09:10
kimorris27
kimorris27 previously approved these changes Oct 24, 2023
@nwnt nwnt added next-release To be included in the next RP release rollout ready-for-review labels Oct 25, 2023
kimorris27
kimorris27 previously approved these changes Oct 25, 2023
@github-actions github-actions bot added needs-rebase branch needs a rebase and removed ready-for-review labels Oct 26, 2023
@github-actions
Copy link

Please rebase pull request.

Copy link
Collaborator

@cadenmarchese cadenmarchese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for addressing the comments!

Copy link
Member

@petrkotas petrkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is a good work. I like the fact that it opens the door to make the other monitors run concurently.

I am tentative approving this one. Good work and I do not see anything that would break exiting code.

c.Monitor(ctx)
monitors = append(monitors, c)
allJobsDone := make(chan bool)
go execute(ctx, allJobsDone, &wg, monitors)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we essentially introduce concurent mechanism to run multiple monitors.
We might as well migrate the rest to use this pattern, because sequential run we have in the original code is error prone.

Copy link
Member

@petrkotas petrkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is a good work. I like the fact that it opens the door to make the other monitors run concurently.

I am tentative approving this one. Good work and I do not see anything that would break exiting code.

@cadenmarchese cadenmarchese dismissed bennerv’s stale review October 27, 2023 13:51

Comments addressed :)

@cadenmarchese cadenmarchese merged commit a6ce8b6 into master Oct 27, 2023
19 checks passed
@nwnt nwnt deleted the nsg-monitoring branch October 27, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-release To be included in the next RP release rollout ready-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants